Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add ability to use latest audio and subtitles #4514

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

carlosjfcasero
Copy link

Changes
Add ability to remember audio and subtitle languages from last played episode. By enabling correspondent option in settings section, every new episode played will take into account last played episode to select audio and/or subtitles track.

@carlosjfcasero carlosjfcasero force-pushed the feat/remember_prev_audio_and_subs branch from 75c568e to 1546856 Compare March 11, 2025 10:16
@johnpc
Copy link

johnpc commented Mar 15, 2025

Looks like an alternative to waiting for #3754, which appears stale

@nielsvanvelzen
Copy link
Member

Just like #3754, this shouldn't need preferences in the client but could be default behavior. Although to be fair, I believe this behavior would be better suited for the server. We already ask it for the (subtitle/audio) streams to use and it already knows we played a previous episode of the same series with a specific subtitle/audio stream. If the change was made on the server it would result in a better experience across all clients.

@carlosjfcasero
Copy link
Author

carlosjfcasero commented Mar 16, 2025

I added preferences:

  1. To let the user choose
  2. Because this part of the code is a little tricky to me (plus there are no tests), so I'm not completely sure I'm not breaking anything (although I have tested it manually).

If you prefer, I can remove preferences and let it as default behavior.
I agree this should be come from the server. However, a lot of issues has been raised (including this one open by myself, which, btw, has been tagged as "Wrong repo") and there is no solution yet (in web it seems to partially work). So, until it's solved server side, I guess client should be able to handle it

What do you think @nielsvanvelzen?

@ConnorS1110
Copy link
Contributor

(If a non-server approach is used) this looks a lot better than what I had in #3754 . I struggled getting playback to actually play, but this looks like it doesn't have the issue I did. I do think I could get behind the server approach for this though. Makes more sense to have the server calculate this in one place the consuming clients could use

@mschumacher69
Copy link

mschumacher69 commented Mar 24, 2025

I added preferences:

  1. To let the user choose
  2. Because this part of the code is a little tricky to me (plus there are no tests), so I'm not completely sure I'm not breaking anything (although I have tested it manually).

If you prefer, I can remove preferences and let it as default behavior. I agree this should be come from the server. However, a lot of issues has been raised (including this one open by myself, which, btw, has been tagged as "Wrong repo") and there is no solution yet (in web it seems to partially work). So, until it's solved server side, I guess client should be able to handle it

What do you think @nielsvanvelzen?

Yeah default behavior would be better, no one would watch an episode in 1 language and another episode in another language, once set, it should stick by default. And I do agree that this is better implemented server side (per user), but if this is the only solution right now, I'd take it as long as this change is reverted once/if a server side solution is implemented later.

@carlosjfcasero carlosjfcasero force-pushed the feat/remember_prev_audio_and_subs branch 2 times, most recently from 3cbcb6a to 0b64ebc Compare March 28, 2025 16:44
@carlosjfcasero carlosjfcasero force-pushed the feat/remember_prev_audio_and_subs branch from 0b64ebc to fef9711 Compare March 28, 2025 16:45
@carlosjfcasero
Copy link
Author

Set as default options. Also created jellyfin/jellyfin#13794 to use it once it's solved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants